Conversation
ClausKlein
requested review from
camio,
neatudarius and
steve-downey
as code owners
| detail/stl_interfaces/config.hpp | ||
| detail/stl_interfaces/fwd.hpp | ||
| detail/stl_interfaces/iterator_interface.hpp) | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/iterator_interface.hpp |
There was a problem hiding this comment.
CMAKE_CURRENT_SOURCE_DIR is redundant for a FILE_SET. It's the root for the source for files in the set.
|
|
||
| target_link_libraries( | ||
| beman.iterator_interface.tests PRIVATE beman::iterator_interface GTest::gtest | ||
| beman.iterator_interface.tests PRIVATE beman::iterator_interface # XXX GTest::gtest |
There was a problem hiding this comment.
The tests have a direct dependency on gtest, not just on gtest main.
There was a problem hiding this comment.
GTest::main links public to Test::getest
There was a problem hiding this comment.
Right, but if we changed the test main it would break the tests. This is the linking equivalent of Include What You Use; relying on transitive links of direct dependencies means surprising build breaks in the future.
There was a problem hiding this comment.
If you want, no problem.
| "cacheVariables": { | ||
| "CMAKE_BUILD_TYPE": "Debug", | ||
| "CMAKE_CXX_FLAGS": "-fsanitize=address -fsanitize=pointer-compare -fsanitize=pointer-subtract -fsanitize=leak -fsanitize=undefined" | ||
| "CMAKE_BUILD_TYPE": "Debug" |
There was a problem hiding this comment.
Is there a target that runs a sanitized build in the presets?
There was a problem hiding this comment.
No, but this preset does not work on APPLE!
There was a problem hiding this comment.
And attaching the sanitizer to the Debug build misses many of the things it is supposed to catch, in any case. And, in addition, specializing for the toolchain is what toolchain files are for, not the Presets.
OK. I wish it were otherwise, but I understand why the change is being requested.
CMakeLists.txt
Outdated
| ${PROJECT_BINARY_DIR}/include/beman/iterator_interface/config.hpp | ||
| ) | ||
|
|
||
| # target_include_directories( |
Cleanup and format
|
Will merge in morning if there are no objections from anyone. |
3 participants
Use FILE_SET HEADERS right